fix(storage): enforce local single-writer ownership - #2079
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens MemPalace’s local-backend safety guarantees by enforcing a single process-lifetime writer owner for file-backed (and unknown) backends, preventing long-lived co-writer processes from corrupting SQLite/WAL/FTS/vector state.
Changes:
- Introduces a backend-level policy (
backend_requires_single_writer) to conservatively require single-writer ownership for local/unknown backends while allowing service-managed concurrency forqdrant/pgvector. - Makes the daemon acquire and hold the palace writer lease for its full lifetime, refusing startup if another writer already owns it.
- Makes writable MCP HTTP acquire the writer lease before binding, refusing startup early when ownership can’t be established; keeps read-only modes available alongside the active writer.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
mempalace/palace.py |
Adds backend classification helper to determine when single-writer enforcement is required. |
mempalace/mcp_server.py |
Tightens writer-lease acquisition semantics, disallows “fail open” writes, and enforces pre-bind ownership for writable HTTP. |
mempalace/daemon.py |
Acquires and holds the palace lease across the daemon’s lifetime for single-writer backends; improves startup refusal behavior and state restoration. |
tests/test_mcp_server.py |
Updates/extends tests for refusal-on-lock-failure and for peer-writer override behavior (local vs remote backends). |
tests/test_mcp_http_transport.py |
Adds coverage ensuring writable HTTP refuses startup without a writer lease while read-only HTTP skips lease acquisition. |
tests/test_daemon.py |
Adds a lifecycle test verifying the daemon holds and releases the writer lease appropriately. |
docs/write-routing-policy.md |
Documents the single-writer model and safe operational routing/recovery for local backends. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d8fc9e66d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _drain_and_cleanup(runtime, palace_path, previous_env) | ||
| finally: | ||
| os.umask(prev_umask) | ||
| writer_lease.close() |
There was a problem hiding this comment.
When a mine or sync job runs longer than SHUTDOWN_DRAIN_SECONDS, _drain_and_cleanup returns while the daemon worker is still executing, and this line releases the writer lease immediately. A new daemon, MCP server, or direct writer can then acquire the lease and mutate the same local backend concurrently with the timed-out worker; this is deterministic when run_server is hosted in an existing process and also creates a shutdown race in the normal daemon process. Keep ownership until the worker has actually stopped or until process termination so shutdown cannot expose a still-active writer.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
fatkobra
left a comment
fatkobra
left a comment
There was a problem hiding this comment.
Thanks for tackling this. The process-lifetime ownership direction is the
right one, and acquiring writable MCP HTTP ownership before binding is a
meaningful improvement.
I am requesting changes because the current patch does not yet enforce the
stated single-writer invariant across all local-backend access paths.
1. Keep daemon ownership while a timed-out worker is still alive
The existing Codex comment is correct.
_drain_and_cleanup() waits only for SHUTDOWN_DRAIN_SECONDS. If the worker
is still executing after that bounded join, run_server() nevertheless
closes writer_lease. A new daemon, MCP process, or direct writer can then
acquire the palace lock while the original worker continues mutating the
backend.
Please retain ownership until the worker has actually stopped or until the
process terminates.
The regression test should block a worker beyond the drain timeout, initiate
shutdown, and verify that a contender still receives MineAlreadyRunning
until the original worker truly exits.
2. The closet-regen-full path from #2045 still bypasses the palace lease
closet_llm.regenerate_closets() opens the drawers and closets collections
directly and performs its purge/upsert cycle under only:
with mine_lock(source):It never contends for mine_palace_lock(palace_path).
That means the exact long-running closet-regeneration writer identified in
#2045 can still run concurrently with the daemon or writable MCP owner on a
local backend. The per-source lock prevents two operations on one source
from interleaving, but it does not establish exclusive ownership of the
palace.
Please make the complete non-dry-run regeneration operation participate in
the same palace ownership policy. The lease should be acquired before
opening the local-backend collections and before making potentially
expensive LLM calls, so a conflicting process fails immediately rather than
running for an extended period before reaching the writes.
mine_palace_lock is process-wide and re-entrant, so this can safely execute
inside the daemon or MCP owner process without conflicting with itself.
A regression test should:
- hold the palace lease in process A;
- invoke
regenerate_closets()forsqlite_exactin process B; - verify that process B refuses before the LLM, delete, or upsert path is
reached.
Please also audit other direct local-backend write entry points rather than
assuming that daemon and MCP startup ownership covers every writer.
3. Read-only MCP is not storage-read-only for sqlite_exact
The PR allows read-only HTTP and stdio readers to coexist with the active
writer, but an actual read eventually calls _get_collection(create=False).
For sqlite_exact, create=False still opens a normal read/write SQLite
connection and always executes _init_schema(). That includes
PRAGMA journal_mode=WAL, DDL, an FTS-table creation attempt, a metadata
upsert, and a commit.
Therefore, a process presented as read-only can still create a write
transaction while another process owns the lifetime writer lease.
The new read-only HTTP test only proves that lease acquisition is skipped
and the HTTP server starts. It does not execute a real read or open the
backend.
Please either:
- add a genuinely read-only backend-open path for
sqlite_exact, using a
read-only SQLite URI or query-only connection and skipping schema
initialization, migrations, and metadata writes; or - do not permit read-only
sqlite_exactprocesses to coexist with the
writer until such an open path exists.
The regression coverage should execute a real read tool against
sqlite_exact, not only mock _serve_http, and prove that opening the
collection performs no schema or metadata mutation.
Once these ownership gaps and the shutdown race are addressed, the overall
approach looks sound.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6e87831bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| collection_name=_config.collection_name, | ||
| create=create, | ||
| backend=backend_name, | ||
| read_only=_READ_ONLY, |
There was a problem hiding this comment.
When a normal writable-capable stdio MCP runs beside a daemon or writable HTTP owner using sqlite_exact, _READ_ONLY is false, so every read tool opens the normal connection here. SQLiteExactBackend._connect() now acquires mine_palace_lock() around schema initialization, which raises while the peer owns the lifetime lease; _get_collection() catches that failure and the read tool cannot recall any drawers. Use a read-only connection while stdio lacks ownership, then discard and reopen it when promotion succeeds, so the documented read coexistence actually works.
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| if not _READ_ONLY: | ||
| writer_ok, writer_reason = _acquire_mcp_writer_lock() |
There was a problem hiding this comment.
When _run_http_loop() is embedded in a process and _serve_http() returns—such as after its caught KeyboardInterrupt, or after the host catches a bind-time SystemExit—this newly unconditional acquisition remains stored in _MCP_WRITER_LOCK_CM and is released only by the atexit callback. The stopped HTTP server therefore continues blocking every writer in other processes until the entire host exits; wrap the serving lifetime in cleanup that exits and clears the lease after active requests have stopped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52d5128d09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _MCP_WRITER_LOCK_FAILED: | ||
| return True, _MCP_WRITER_LOCK_ERROR | ||
| return False, _MCP_WRITER_LOCK_ERROR |
There was a problem hiding this comment.
When a writable stdio server encounters a recoverable setup error—such as a temporary backend-artifact mismatch or lock-directory permission failure—the generic exception path sets _MCP_WRITER_LOCK_FAILED, and this early return permanently refuses every later mutating request. Even after the operator corrects the filesystem state, the same process cannot recover, and mempalace_reconnect is also rejected by this guard. Cache only genuinely permanent failures, or retry setup so corrected environments can resume writes without restarting the MCP host.
Useful? React with 👍 / 👎.
Summary
Root cause
The reported MCP HTTP + daemon serve + external mine topology could create multiple independent long-lived clients for the same local SQLite-backed palace:
MEMPALACE_MCP_ALLOW_PEER_WRITER=1bypassed protection for local file-backed backendsSerializing individual calls is insufficient when separate processes retain SQLite/WAL, FTS, or vector-index state between operations.
Behavior after this change
chroma,sqlite_exact, Milvus Lite, and unknown/plugin backends conservatively require one writer process per palaceqdrantandpgvectorretain service-managed multi-process concurrencyNo unsafe lock unlinking or reclamation is introduced. Operators should stop the owning process cleanly and perform integrity or repair work offline.
Validation
Focused Ruff checks and
git diff --checkalso pass.Fixes #2045